Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use native crc32 on x86 if available. #8169

Closed
wants to merge 2 commits into from

Conversation

Teselka
Copy link
Contributor

@Teselka Teselka commented Nov 20, 2024

This adds support for the native _mm_crc32_u8 on x86 provided by the SSE4.2 instruction set (please note that this one actually is crc32c, which uses a different polynomial that is used internally by the Dear ImGui currently).

Because it's a native instruction, it doesn't require a 1 kb lookup table.

@Teselka
Copy link
Contributor Author

Teselka commented Nov 20, 2024

Oh, i forgot to see if anyone did this before #4933

@ocornut
Copy link
Owner

ocornut commented Nov 20, 2024

Hello, thanks for the PR! This looks simpler than #4933.

which uses a different polynomial that is used internally by the Dear ImGui currently).

I would want to tackle this first.
What would it take to change the regular C++ implementation to have output matching the use of _mm_crc32_u8() ? (and potentially of wider versions _mm_crc32_u32 if we decide it is worth using them).

@Teselka
Copy link
Contributor Author

Teselka commented Nov 22, 2024

Hello, thanks for the PR! This looks simpler than #4933.

which uses a different polynomial that is used internally by the Dear ImGui currently).

I would want to tackle this first. What would it take to change the regular C++ implementation to have output matching the use of _mm_crc32_u8() ? (and potentially of wider versions _mm_crc32_u32 if we decide it is worth using them).

That would require to change only current crc lookup table

@Teselka
Copy link
Contributor Author

Teselka commented Nov 22, 2024

Changed it, now it produces same results with or without native instruction.

ocornut pushed a commit that referenced this pull request Nov 27, 2024
@ocornut
Copy link
Owner

ocornut commented Nov 27, 2024

This is merged (in reverse order) as e6dd8f6 + 326dc95
I also updated test engine to match. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants